Skip to content

wallet: Remove mainchain tip check on NeedsAccountsSync#2327

Merged
jrick merged 1 commit intodecred:masterfrom
matheusd:acct-discovery-partial-download
Feb 6, 2024
Merged

wallet: Remove mainchain tip check on NeedsAccountsSync#2327
jrick merged 1 commit intodecred:masterfrom
matheusd:acct-discovery-partial-download

Conversation

@matheusd
Copy link
Copy Markdown
Member

@matheusd matheusd commented Feb 2, 2024

This check could prevent a wallet from going through address discovery if it had not completed the initial chain sync prior to being restarted.

One instance this could happen is when restoring SPV wallets under an instable network connection or having a power loss during the initial sync of a restored wallet.

Detected while debugging issues with #2318.

@jrick
Copy link
Copy Markdown
Member

jrick commented Feb 2, 2024

I'm pretty sure i've relied on this as a hack to get around the initial discovery when i planned to avoid it anyways by manually setting the address indexes, e.g. on a voter with mixed ticketbuying and large gaps.

Seems fine to fix this, but I do think we want a proper way to intentionally opt out, probably with some additional prompts during wallet creation or more flags.

@jrick
Copy link
Copy Markdown
Member

jrick commented Feb 2, 2024

Wait, hold on. This is for account discovery, not address discovery.

The only location this is used in dcrwallet itself (not importing an embedded wallet) is https://github.com/decred/dcrwallet/blob/master/dcrwallet.go#L485-L500, and all this does is cause the wallet to be unlocked at the time of discovery (it will run regardless of what this function returns or not), and because the wallet is unlocked, it will opportunistically also discover accounts and not just addresses.

@matheusd
Copy link
Copy Markdown
Member Author

matheusd commented Feb 5, 2024

Seems fine to fix this, but I do think we want a proper way to intentionally opt out, probably with some additional prompts during wallet creation or more flags.

You mean, opt out of address/account discovery on a given wallet? Something like --noaccountdiscovery and/or --noaddressdiscovery? If so, I can these on this PR.

@jrick
Copy link
Copy Markdown
Member

jrick commented Feb 5, 2024

They aren't needed on this pr, but yeah flags like that would be nice.

This still looks like a bug worth fixing as is, but I just don't think the explanation is accurate. Is some other software e.g. dcrlnd using this return result to trigger address discovery?

@matheusd
Copy link
Copy Markdown
Member Author

matheusd commented Feb 5, 2024

No, you are correct this is regarding account discovery, not address discovery. I'll fix the description in the commit.

This check could prevent a wallet from going through account discovery
if it had not completed the initial chain sync prior to being restarted.

One instance this could happen is when restoring SPV wallets under an
instable network connection or having a power loss during the initial
sync of a restored wallet.
@matheusd matheusd force-pushed the acct-discovery-partial-download branch from 796fb76 to 2264d0f Compare February 6, 2024 10:42
@jrick jrick merged commit 2264d0f into decred:master Feb 6, 2024
@matheusd matheusd deleted the acct-discovery-partial-download branch February 6, 2024 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants